Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added constraints to Nomad Cloud plugin #17

Merged
merged 2 commits into from
Jan 5, 2018
Merged

Conversation

scarvel8
Copy link

This adds the ability to impose constraints on a job submitted to Nomad via the Nomad Cloud plugin.

https://www.nomadproject.io/docs/job-specification/constraint.html

This plugin uses the JSON form to submit jobs (https://www.nomadproject.io/docs/http/json-jobs.html), therefore it uses the LTarget, RTarget, and Operand form to impose constraints. ${ } is automatically wrapped around the LTarget as most job constraints would use this parameter.

For example, to restrict to linux machines, you would add to the constraint field:
attr.kernel.name,windows,=

Items are comma separated, groups of three are required, the field is optional.

@@ -16,6 +16,10 @@
<f:textbox/>
</f:entry>

<f:entry title="${Constraints}" field="constraints" description="comma separated LTarget,RTarget,Operand">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use LTarget,Operand,RTarget which is way more natural to scan

also, how do you add multiple constraints? using ; as sep?

Copy link

@jippi jippi Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it also work (if at all) for distinct_hosts?

if Operand is distinct_host the [https://www.nomadproject.io/docs/http/json-jobs.html](nomad json docs) says When specified, LTarget and RTarget should be omitted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and distinct_property says When specified, LTarget should be the property that should be distinct and and RTarget should be omitted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using comma may interfere with set_contains with assume RTarget is a comma separated list of values

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, LTarget,Operand,RTarget looks better. Multiple constraints at the moment are separated automatically:

1,=,2,3,=,4
Makes two constraints: 1=2 and 3=4

Copy link
Author

@scarvel8 scarvel8 Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently will not work for distinct_host because I am automatically wrapping the LTarget with ${ } as Jenkins does not let you input these characters into a text field easily. I will change the code so LTarget is blank if not specified, this would allow distinct_host to work if you did something like:
Input: ,distinct_host,
However, this would not fix the distinct_property problem as it requires an LTarget (which will automatically be wrapped with ${ }), unless I use an if statement to check for distinct_property, but that is not very future-proof

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the comma, I should use a different delimiter. It is hard to decide the "best" way to input the data into the field, as the three main challenges are:

1.) Need to input N groups of 3 items in a single textbox
2.) Textbox filters out certain characters (i.e. ${}, which Nomad requires in the LTarget for certain constraints)
3.) Cannot use comma as a separator

I'll wait a few days to see If there are some good suggestions for how to input the text into this field, I can implement them. Otherwise, I'll go ahead and come up with something on my own (might not be very user friendly or intuitive, though.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ltarget,operand,rtarget|ltarget,operand,rtarget
ltarget,operand,rtarget/ltarget,operand,rtarget
ltarget,operand,rtarget:ltarget,operand,rtarget
ltarget,operand,rtarget#ltarget,operand,rtarget

is my suggestions, not sure if they are brilliant, the best would be to have a line per constraint, can jenkins do multiple inputs ? |,/, :, #

Copy link
Author

@scarvel8 scarvel8 Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the commas interfere with set_contains, maybe it would be better to do something like:
ltarget operand rtarget|ltarget operand rtarget
I also need to find some way to be able to deal with ${ } as well, I will take a look again to see if its possible to accept the input for ${ } directly in the textbox, or maybe prepend ltarget with a character such as - to allow the expansion to happen, and to keep things somewhat future-proof.
Spaces are not really a great separator either in the case of ltarget or rtarget being omitted

@jippi
Copy link

jippi commented Mar 16, 2017

The code looks good, though we should either document the caveats of some features, probably, not working atm - that said, having basic constraints working is better than no constraints at all !

@scarvel8
Copy link
Author

Working on an update that will cover all contraints and be a lot more friendly in the UI, stay tuned :)

@jippi
Copy link

jippi commented Mar 24, 2017

Exciting stuff @scarvel8 !

@jippi
Copy link

jippi commented Apr 8, 2017

did this stall @scarvel8 ? :)

@scarvel8
Copy link
Author

Implemented the GUI changes and it works when saved, but had some problems getting values to recall back into the GUI when you re-enter Configure System. Hopefully will get the chance to work on this by the end of the week

@jippi
Copy link

jippi commented Apr 11, 2017

Nice! good job!

@scarvel8
Copy link
Author

Updated PR and tested, usage should be pretty straightforward (this version allows symbols in the text fields and uses a Jelly repeatable to be able to add and remove constraints easily from the GUI). @jippi

@antweiss
Copy link

Not finding the time to test this.
Anyone volunteering to test and confirm it works?

@jippi
Copy link

jippi commented Jul 14, 2017

@scarvel8 what did the status for this PR end with? stable?

@zstyblik
Copy link

Hello, are there plans to merge this?

@jovandeginste
Copy link

@jippi @scarvel8 I built and installed this fork in our production environment. I will start using it for production load today and report back. First tests and impressions seem to indicate it works as expected.

@jovandeginste
Copy link

Seems to work so far with some production load.

@scarvel8
Copy link
Author

scarvel8 commented Oct 4, 2017

@jovandeginste sounds good, @jippi it has been working for me for my minor use case, but I admit I have not tested exhaustively

@zstyblik
Copy link

It works for me as well, although I have only one constrain so far :)

@jovandeginste
Copy link

Had no issues so far, lgtm

@scarvel8
Copy link
Author

scarvel8 commented Jan 2, 2018

Any reason against merging this?

@zstyblik
Copy link

zstyblik commented Jan 5, 2018

@jippi ?

@jippi jippi merged commit d9ab01a into jenkinsci:master Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants